-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(Closes #2499) scalarization transformation implementation #2563
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2563 +/- ##
========================================
Coverage 99.89% 99.89%
========================================
Files 359 360 +1
Lines 51102 51281 +179
========================================
+ Hits 51050 51229 +179
Misses 52 52 ☔ View full report in Codecov by Sentry. |
@hiker Quick question on how the ordering of statements goes with the dependency system - if you have an assignment such as |
@sergisiso this is ready for a first look. I'm waiting for hiker to clarify my question before I remove the code at lines 123 in the transformation, but I expect to remove those lines as I think they're unreachable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good initial implementation @LonelyCat124, I am just asking if the code can be slightly refactored to be more readable and I am wondering if some logic should leave inside the next_access methods, I am happy to discuss this one.
@sergisiso I've addressed most of the comments here - I think the next_access one is a bigger issue than just here (and we need to decide if the variable access tooling should handle this or if we just do it in next_access, but I think we can do an implementation for this in the mean time anyway). I need to still expand the tests for the apply routine and add documentation before another review though I think. |
@LonelyCat124 I don't mind the order of these (if you put the associated TODOs), but wouldn't want to go very far with some logic that then will need to be deleted. |
I brought this up to master now and made the tests pass, but I'll start reworking the logic here now. |
Setting the integration tests going one more time, but assumping they're ok this will be ready for review. |
@sergisiso this is ready for review now - only remaining thing I know I need to change is to revert the transformation script change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @LonelyCat124 this is a complex transformation but I like its thorough testing. Its also good to see the DefUseChain tested in real NEMO code, that will be useful for upcoming work.
The remaining comments are mostly about cleaning up comments and documentation.
src/psyclone/tests/psyir/tools/definition_use_chains_forward_dependence_test.py
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/scalarization_trans_test.py
Outdated
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/scalarization_trans_test.py
Outdated
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/scalarization_trans_test.py
Outdated
Show resolved
Hide resolved
src/psyclone/tests/psyir/transformations/scalarization_trans_test.py
Outdated
Show resolved
Hide resolved
@sergisiso I've fixed the changes for the review now so back to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LonelyCat124 All comments have been addressed, I fixed a couple of small things with the passthrough and updated the version since it is the first PR after a release. I will launch the tests once more since we touched the scripts although Scalarization should be disable by default.
While updating the changelog I also realised that we use "Scalarization" while other parts of the code use the "ise" instead of "ize". @LonelyCat124 @arporter should this be renamed as Scalarisation or is it fine as it is?
I wasn't sure about the s vs z for this. I think probably british english should be sation but the US spelling is much more commonly used for this but happy to change it if @arporter would prefer also. |
I would prefer to keep with the British spelling :-) |
ok I've done a global s/calariz/calaris/ that I think should cover all instances. At least grepping I can't find anything wrong and still passes all tests. |
Ah I just did that too - I will not push then. |
First implementation is here. I have unit tests for the private routines, I need to test it with code examples doing the full transformation properly but in theory its implemented here for now.